feat(auth): Improve auth error messaging with ErrorBuilder pattern#1851
feat(auth): Improve auth error messaging with ErrorBuilder pattern#1851
Conversation
milldr
added
the
no-release
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1851 +/- ##
==========================================
+ Coverage 73.69% 73.71% +0.01%
==========================================
Files 736 737 +1
Lines 67269 67596 +327
==========================================
+ Hits 49577 49830 +253
- Misses 14321 14392 +71
- Partials 3371 3374 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR implements structured error messaging for Atmos authentication by introducing an ErrorBuilder pattern across the auth subsystem. It adds formatting helpers for profile and expiration display, updates error chain traversal to aggregate context across layers, and replaces ad-hoc error wrapping with enriched errors containing explanations, hints, and contextual fields throughout the manager, hooks, and command implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes follow a consistent ErrorBuilder pattern applied repetitively across the auth subsystem, reducing heterogeneity. However, the scope spans multiple files with new helper functions, enhanced error chain logic, and wide-ranging error path updates. The homogeneous nature of the pattern replacements moderates the overall review complexity. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/prd/auth-error-messaging.md (3)
11-11: Add language identifiers to all fenced code blocks.Markdown linting flagged missing language specifiers, which affects syntax highlighting and rendering consistency. Add appropriate language tags to code blocks.
-``` +```text Error: authentication failed: failed to authenticate via credential chain for identity "plat-dev/terraform": authentication failed: identity=plat- dev/terraform step=1: authentication failed -``` +```Apply similar changes for all code blocks—add
yamlfor YAML blocks (lines 77, 183-189, 220-226),gofor Go blocks (lines 130-139, 280-297), andmarkdownortextfor example output blocks (lines 205, 242).Also applies to: 77-77, 95-95, 101-101, 107-107, 205-205, 242-242
64-64: Add blank lines before tables for proper markdown formatting.Tables should be surrounded by blank lines per markdownlint rules. Add a blank line before the table at line 64 (R1 table), line 204 (Example 1 Context table), and line 241 (Example 2 Context table).
| Context | Description | Example | | `identity` | Target identity name | `plat-dev/terraform` |Also applies to: 204-204, 241-241
192-204: Clarify or restructure duplicate "Context" headings in examples.The Examples section repeats "## Context" and "## Hints" headings across Examples 1, 2, and 3 (lines 192-204, 229-241, 264-272). This is flagged by markdownlint as duplicate headings. While the repetition may be intentional to show consistent error output format, consider:
- Option A (preserve current): Use subheadings like
### Contextor### Hintswithin each example to clarify they're part of the example template.- Option B (restructure): Move the example output into fenced code blocks labeled as "Example Output" to make the structure clearer.
This is a minor documentation clarity issue—no functional impact.
Also applies to: 229-241
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/prd/auth-error-messaging.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for PRD filenames:
command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md. All PRDs indocs/prd/
Files:
docs/prd/auth-error-messaging.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
🪛 LanguageTool
docs/prd/auth-error-messaging.md
[typographical] ~70-~70: Consider using a typographic opening quote here.
Context: ...er| |profiles| Active profiles or "(not set)" |ATMOS_PROFILE=devops` | | ...
(EN_QUOTES)
[typographical] ~70-~70: Consider using a typographic close quote here.
Context: ...rofiles| Active profiles or "(not set)" |ATMOS_PROFILE=devops| |chain_ste...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic opening quote here.
Context: ...----|----------------| | Auth failure | "Run atmos auth --help for troubleshoot...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic close quote here.
Context: ... atmos auth --help for troubleshooting" | | Expired credentials | "Run `atmos a...
(EN_QUOTES)
[typographical] ~118-~118: Consider using a typographic opening quote here.
Context: ...ubleshooting" | | Expired credentials | "Run atmos auth login to refresh creden...
(EN_QUOTES)
[typographical] ~118-~118: Consider using a typographic close quote here.
Context: ...atmos auth loginto refresh credentials" | | Missing credentials | "Runatmos a...
(EN_QUOTES)
[typographical] ~119-~119: Consider using a typographic opening quote here.
Context: ... credentials" | | Missing credentials | "Run atmos auth login to authenticate" ...
(EN_QUOTES)
[typographical] ~119-~119: Consider using a typographic close quote here.
Context: ... "Run atmos auth login to authenticate" | | SSO session expired | "Run `atmos a...
(EN_QUOTES)
[typographical] ~120-~120: Consider using a typographic opening quote here.
Context: ...authenticate" | | SSO session expired | "Run atmos auth login --provider <name>...
(EN_QUOTES)
[typographical] ~120-~120: Consider using a typographic close quote here.
Context: ...in --provider to re-authenticate" | | Identity not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~121-~121: Consider using a typographic opening quote here.
Context: ...-authenticate" | | Identity not found | "Run atmos list identities to see avail...
(EN_QUOTES)
[typographical] ~121-~121: Consider using a typographic close quote here.
Context: ... identitiesto see available identities" | | Provider not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~122-~122: Consider using a typographic opening quote here.
Context: ...le identities" | | Provider not found | "Run atmos list providers to see availa...
(EN_QUOTES)
[typographical] ~122-~122: Consider using a typographic close quote here.
Context: ...st providers` to see available providers" | | No profile set | "Set ATMOS_PROFILE...
(EN_QUOTES)
[typographical] ~123-~123: Consider using a typographic opening quote here.
Context: ...ailable providers" | | No profile set | "Set ATMOS_PROFILE or use --profile flag"...
(EN_QUOTES)
[typographical] ~123-~123: Consider using a typographic close quote here.
Context: ..."Set ATMOS_PROFILE or use --profile flag" | | Circular dependency | "Check identi...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...profile flag" | | Circular dependency | "Check identity chain configuration for c...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ... identity chain configuration for cycles" | ### R5: ErrorBuilder Migration All ...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/auth-error-messaging.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
229-229: Multiple headings with the same content
(MD024, no-duplicate-heading)
234-234: Multiple headings with the same content
(MD024, no-duplicate-heading)
241-241: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (3)
docs/prd/auth-error-messaging.md (3)
328-335: Verify external documentation references exist.Lines 330-333 reference documentation files using relative paths:
../errors.md../auth/README.md./atmos-profiles.md./auth-default-settings.mdPlease confirm these files exist and the paths are correct. If any are stale or don't exist yet, update references or add a note about file location assumptions.
58-162: Requirements section is comprehensive and well-structured.The six requirements (R1–R6) clearly define error context, display formats, the ErrorBuilder pattern, and sentinel error categories. The tables in R4 (Actionable Hints) and R6 (Error Categories) are particularly helpful for implementation guidance. The Go code example in R5 shows the intended API pattern clearly.
168-272: Examples are concrete and cover key scenarios well.Three examples effectively illustrate:
- Auth failure with profile set
- Expired credentials with no profile
- Identity not found in profile
Each shows the proposed output format, hints, and context. This provides good implementation guidance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/prd/auth-error-messaging.md (2)
71-71: Clarify whatchain_stepshould convey about authentication failure.A past review noted concern about the
chain_steprepresentation. Currently showing "2 of 3" indicates position but doesn't clarify where in the authentication chain the failure occurred. Consider whether this should include information about which step failed (e.g., "step 2 of 3: github-oidc assumption failed") or if the explanation field handles that detail.
239-240: Add blank lines around table for markdown compliance.Tables should be surrounded by blank lines per markdown standards. Insert a blank line before line 239 and after the code block ends.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/prd/auth-error-messaging.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for PRD filenames:
command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md. All PRDs indocs/prd/
Files:
docs/prd/auth-error-messaging.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
🪛 LanguageTool
docs/prd/auth-error-messaging.md
[typographical] ~70-~70: Consider using a typographic opening quote here.
Context: ...er| |profiles| Active profiles or "(not set)" |ATMOS_PROFILE=devops` | | ...
(EN_QUOTES)
[typographical] ~70-~70: Consider using a typographic close quote here.
Context: ...rofiles| Active profiles or "(not set)" |ATMOS_PROFILE=devops| |chain_ste...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...----|----------------| | Auth failure | "Run atmos auth --help for troubleshoot...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ... atmos auth --help for troubleshooting" | | Expired credentials | "Run `atmos a...
(EN_QUOTES)
[typographical] ~125-~125: Consider using a typographic opening quote here.
Context: ...ubleshooting" | | Expired credentials | "Run atmos auth login to refresh creden...
(EN_QUOTES)
[typographical] ~125-~125: Consider using a typographic close quote here.
Context: ...atmos auth loginto refresh credentials" | | Missing credentials | "Runatmos a...
(EN_QUOTES)
[typographical] ~126-~126: Consider using a typographic opening quote here.
Context: ... credentials" | | Missing credentials | "Run atmos auth login to authenticate" ...
(EN_QUOTES)
[typographical] ~126-~126: Consider using a typographic close quote here.
Context: ... "Run atmos auth login to authenticate" | | SSO session expired | "Run `atmos a...
(EN_QUOTES)
[typographical] ~127-~127: Consider using a typographic opening quote here.
Context: ...authenticate" | | SSO session expired | "Run atmos auth login --provider <name>...
(EN_QUOTES)
[typographical] ~127-~127: Consider using a typographic close quote here.
Context: ...in --provider to re-authenticate" | | Identity not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~128-~128: Consider using a typographic opening quote here.
Context: ...-authenticate" | | Identity not found | "Run atmos list identities to see avail...
(EN_QUOTES)
[typographical] ~128-~128: Consider using a typographic close quote here.
Context: ... identitiesto see available identities" | | Provider not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~129-~129: Consider using a typographic opening quote here.
Context: ...le identities" | | Provider not found | "Run atmos list providers to see availa...
(EN_QUOTES)
[typographical] ~129-~129: Consider using a typographic close quote here.
Context: ...st providers` to see available providers" | | No profile set | "Set ATMOS_PROFILE...
(EN_QUOTES)
[typographical] ~130-~130: Consider using a typographic opening quote here.
Context: ...ailable providers" | | No profile set | "Set ATMOS_PROFILE or use --profile flag"...
(EN_QUOTES)
[typographical] ~130-~130: Consider using a typographic close quote here.
Context: ..."Set ATMOS_PROFILE or use --profile flag" | | Circular dependency | "Check identi...
(EN_QUOTES)
[typographical] ~131-~131: Consider using a typographic opening quote here.
Context: ...profile flag" | | Circular dependency | "Check identity chain configuration for c...
(EN_QUOTES)
[typographical] ~131-~131: Consider using a typographic close quote here.
Context: ... identity chain configuration for cycles" | ### R5: ErrorBuilder Migration All ...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/auth-error-messaging.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
239-239: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (2)
docs/prd/auth-error-messaging.md (2)
73-97: Verbose mode handling looks solid.The conditional display of identity configuration only when
--verboseis set directly addresses earlier feedback about not overwhelming every error with excessive detail. This is well-thought-out.
1-368: Structure and requirements are comprehensive.The PRD cleanly separates concerns (authentication failures, credential cache issues, configuration validation), provides clear requirements (R1-R6), practical examples across scenarios, and actionable success criteria. The ErrorBuilder pattern migration scope is well-defined, and the implementation notes include helpful guidance on verbose mode detection and helper functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/prd/auth-error-messaging.md (1)
207-241: Add blank line after table and language specifier to example block.The table at lines 232–239 needs a blank line after it before the next section starts (currently Example 3 begins immediately at line 242). Additionally, the markdown example block (lines 207–240) should be wrapped with a language specifier so markdown linters recognize it:
## Example 2: Authentication Failed (Verbose Mode) With `--verbose` flag, the identity configuration is included: -```markdown +```markdown # Authentication Error **Error:** authentication failed ... | chain_step | 1 of 2 |
Example 3: Credentials Expired (No Profile)
(The outer example block at 207–240 itself needs `markdown` language specifier if not already present in the source.) </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Disabled knowledge base sources:** - Linear integration is disabled by default for public repositories > You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c9064d6ec59467c73f21d4cd665f903d33941eea and 7e32a37c540d4067f1dfb11a24487e73487f5435. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/prd/auth-error-messaging.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>docs/prd/*.md</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Use kebab-case for PRD filenames: `command-registry-pattern.md`, `error-handling-strategy.md`, `testing-strategy.md`. All PRDs in `docs/prd/` Files: - `docs/prd/auth-error-messaging.md` </details> </details><details> <summary>🧠 Learnings (1)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.</details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/prd/auth-error-messaging.md</summary> [typographical] ~70-~70: Consider using a typographic opening quote here. Context: ...er` | | `profiles` | Active profiles or "(not set)" | `ATMOS_PROFILE=devops` | | ... (EN_QUOTES) --- [typographical] ~70-~70: Consider using a typographic close quote here. Context: ...rofiles` | Active profiles or "(not set)" | `ATMOS_PROFILE=devops` | | `chain_ste... (EN_QUOTES) --- [typographical] ~124-~124: Consider using a typographic opening quote here. Context: ...----|----------------| | Auth failure | "Run `atmos auth --help` for troubleshoot... (EN_QUOTES) --- [typographical] ~124-~124: Consider using a typographic close quote here. Context: ... `atmos auth --help` for troubleshooting" | | Expired credentials | "Run `atmos a... (EN_QUOTES) --- [typographical] ~125-~125: Consider using a typographic opening quote here. Context: ...ubleshooting" | | Expired credentials | "Run `atmos auth login` to refresh creden... (EN_QUOTES) --- [typographical] ~125-~125: Consider using a typographic close quote here. Context: ...atmos auth login` to refresh credentials" | | Missing credentials | "Run `atmos a... (EN_QUOTES) --- [typographical] ~126-~126: Consider using a typographic opening quote here. Context: ... credentials" | | Missing credentials | "Run `atmos auth login` to authenticate" ... (EN_QUOTES) --- [typographical] ~126-~126: Consider using a typographic close quote here. Context: ... "Run `atmos auth login` to authenticate" | | SSO session expired | "Run `atmos a... (EN_QUOTES) --- [typographical] ~127-~127: Consider using a typographic opening quote here. Context: ...authenticate" | | SSO session expired | "Run `atmos auth login --provider <name>`... (EN_QUOTES) --- [typographical] ~127-~127: Consider using a typographic close quote here. Context: ...in --provider <name>` to re-authenticate" | | Identity not found | "Run `atmos li... (EN_QUOTES) --- [typographical] ~128-~128: Consider using a typographic opening quote here. Context: ...-authenticate" | | Identity not found | "Run `atmos list identities` to see avail... (EN_QUOTES) --- [typographical] ~128-~128: Consider using a typographic close quote here. Context: ... identities` to see available identities" | | Provider not found | "Run `atmos li... (EN_QUOTES) --- [typographical] ~129-~129: Consider using a typographic opening quote here. Context: ...le identities" | | Provider not found | "Run `atmos list providers` to see availa... (EN_QUOTES) --- [typographical] ~129-~129: Consider using a typographic close quote here. Context: ...st providers` to see available providers" | | No profile set | "Set ATMOS_PROFILE... (EN_QUOTES) --- [typographical] ~130-~130: Consider using a typographic opening quote here. Context: ...ailable providers" | | No profile set | "Set ATMOS_PROFILE or use --profile flag"... (EN_QUOTES) --- [typographical] ~130-~130: Consider using a typographic close quote here. Context: ..."Set ATMOS_PROFILE or use --profile flag" | | Circular dependency | "Check identi... (EN_QUOTES) --- [typographical] ~131-~131: Consider using a typographic opening quote here. Context: ...profile flag" | | Circular dependency | "Check identity chain configuration for c... (EN_QUOTES) --- [typographical] ~131-~131: Consider using a typographic close quote here. Context: ... identity chain configuration for cycles" | ### R5: ErrorBuilder Migration All ... (EN_QUOTES) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/prd/auth-error-messaging.md</summary> 239-239: Tables should be surrounded by blank lines (MD058, blanks-around-tables) --- 240-240: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Acceptance Tests (windows) * GitHub Check: Summary </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>docs/prd/auth-error-messaging.md (2)</summary><blockquote> `1-58`: **Well-scoped problem statement and clear goals.** The problem statement effectively demonstrates the gap (generic error with no context), and the scope is appropriately bounded. The three error categories cover the expected failure modes without overreaching into auth flow changes. --- `59-333`: **Requirements and implementation plan are thorough and actionable.** The ErrorBuilder pattern migration is well-specified, and R2 properly gates verbose-mode identity config display so errors remain concise by default. The helper functions are practical, and the file-by-file migration list gives implementers a clear roadmap. The verbose mode logic aligns nicely with atmos' existing `logs.level: trace` configuration rather than inventing a new flag, which should feel natural to users. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/auth/manager.go (1)
157-162: Consider a more specific error sentinel.Using
ErrNilParamfor an empty identity string works but feels slightly off semantically. An empty string isn't quite the same as nil. That said, the error message is clear enough for users.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/auth/errors.gopkg/auth/errors_test.gopkg/auth/manager.gopkg/auth/manager_extended_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
pkg/auth/errors.gopkg/auth/manager_extended_test.gopkg/auth/errors_test.gopkg/auth/manager.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
pkg/auth/manager_extended_test.gopkg/auth/errors_test.go
🧠 Learnings (16)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/auth/errors.gopkg/auth/manager.go
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
pkg/auth/errors.gopkg/auth/errors_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/auth/errors.gopkg/auth/errors_test.gopkg/auth/manager.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/auth/errors.gopkg/auth/errors_test.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Applied to files:
pkg/auth/errors.gopkg/auth/manager.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/auth/errors.gopkg/auth/errors_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/errors.gopkg/auth/manager_extended_test.gopkg/auth/errors_test.gopkg/auth/manager.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager_extended_test.gopkg/auth/manager.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks (example: pkg/store/)
Applied to files:
pkg/auth/manager_extended_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/auth/errors_test.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
pkg/auth/errors_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/auth/errors_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/auth/errors_test.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager.go
🧬 Code graph analysis (1)
pkg/auth/manager.go (2)
errors/builder.go (1)
Build(24-37)errors/errors.go (7)
ErrNilParam(563-563)ErrIdentityNotFound(568-568)ErrAuthenticationFailed(539-539)ErrPostAuthenticationHookFailed(540-540)ErrProviderNotFound(569-569)ErrNoCredentialsFound(561-561)ErrExpiredCredentials(562-562)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (14)
pkg/auth/manager_extended_test.go (1)
488-489: LGTM!The added mock expectation aligns with the enriched error context introduced in
manager.go. When credentials are expired, the error builder now queriesGetProviderName()to include provider context in the error message.pkg/auth/errors_test.go (1)
1-77: LGTM!Solid table-driven tests covering edge cases for both formatting helpers. The nil/empty/single/multiple profile scenarios and nil/valid expiration paths provide good coverage.
pkg/auth/errors.go (1)
1-24: LGTM!Clean, focused helper functions. Using
len(profiles) == 0correctly handles both nil and empty slices, andtime.RFC3339ensures consistent timestamp formatting across error messages.pkg/auth/manager.go (11)
168-175: Good error enrichment.Clear explanation with two actionable hints. Setting provider to "(not set)" makes sense since the identity wasn't found.
183-192: LGTM!Good approach extracting provider name before constructing the error. The hint about checking for cycles is particularly useful.
201-212: LGTM!Safe chain access with length check before extracting provider name. Error context is well-structured.
240-247: LGTM!Correctly uses
rootProviderNamefrom the successfully built chain.
272-277: LGTM!Clear error with actionable hint to list available providers.
336-342: LGTM!Consistent error pattern with the identity-not-found handling in
Authenticate.
351-364: LGTM!Good defensive coding—attempts to enrich with provider context but gracefully falls back to "(not set)" if
GetProviderNamefails.
370-384: LGTM!Including the expiration timestamp in the error context is a nice touch for debugging. The ignored error from
GetExpiration()is acceptable here since it's for enrichment only.
1286-1292: LGTM!Consistent error pattern maintained across the codebase.
1319-1325: LGTM!Maintains consistency with other identity-not-found error paths.
1371-1379: LGTM!Clean helper with proper nil handling. Nicely encapsulates profile access for error context.
milldr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/auth/manager_test.go (1)
169-200: Consider validating the error context format more explicitly.The test now correctly uses sentinel errors and inspects structured context, but the context string validation could be more robust. The comment on line 179 mentions an expected format
"defaults=charlie, zebra, alpha profile=(not set)", but the test only checks that each identity name appears somewhere in the string.Optional: Add format validation
Consider adding an assertion that the context follows the expected key-value format:
require.NotEmpty(t, contextStr, "error should have defaults context") + // Verify the context string follows the expected format. + assert.Contains(t, contextStr, "defaults=", "context should have defaults key") + assert.Contains(t, contextStr, "profile=", "context should have profile key") + // The context string should contain all default identities. assert.Contains(t, contextStr, "alpha")This would catch regressions if the error context structure changes unexpectedly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/auth/manager.gopkg/auth/manager_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
pkg/auth/manager_test.gopkg/auth/manager.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
pkg/auth/manager_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager_test.gopkg/auth/manager.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Applied to files:
pkg/auth/manager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/auth/manager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/auth/manager_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/manager_test.gopkg/auth/manager.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
pkg/auth/manager.go
🧬 Code graph analysis (2)
pkg/auth/manager_test.go (1)
errors/errors.go (1)
ErrMultipleDefaultIdentities(572-572)
pkg/auth/manager.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (11)
ErrNilParam(563-563)ErrIdentityNotFound(568-568)ErrAuthenticationFailed(539-539)ErrPostAuthenticationHookFailed(540-540)ErrProviderNotFound(569-569)ErrNoCredentialsFound(561-561)ErrExpiredCredentials(562-562)ErrIdentitySelectionRequiresTTY(574-574)ErrNoDefaultIdentity(571-571)ErrMultipleDefaultIdentities(572-572)ErrNoIdentitiesAvailable(573-573)pkg/auth/errors.go (2)
FormatProfile(10-15)FormatExpiration(19-24)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Summary
🔇 Additional comments (11)
pkg/auth/manager_test.go (1)
8-8: LGTM!The new imports support the structured error inspection approach using CockroachDB's error details API and string operations for context parsing.
Also applies to: 12-12
pkg/auth/manager.go (10)
157-162: LGTM!The error enrichment provides clear explanation and actionable hints. The use of
FormatProfile(m.getProfiles())safely handles nil profiles by returning "(not set)".
168-176: LGTM!Error enrichment provides comprehensive context with multiple actionable hints for troubleshooting identity lookup failures.
183-192: LGTM!Error properly wraps the underlying cause and provides specific guidance about chain configuration issues. Provider name resolution before error construction ensures complete context.
201-212: LGTM!Error handling correctly derives provider context from the authentication chain and wraps the underlying failure with comprehensive context.
240-247: LGTM!Post-authentication hook error handling provides clear context with the root provider name and preserves the underlying error cause.
272-277: LGTM!Provider lookup error provides clear guidance with actionable hint to list available providers.
336-342: LGTM!The three error paths in
GetCachedCredentials()follow consistent patterns with appropriate sentinel errors and comprehensive context. The provider name retrieval logic (lines 352-356 and 371-375) is duplicated but acceptable given the small scope.The expired credentials error properly includes formatted expiration time for debugging.
Also applies to: 351-364, 370-384
440-445: LGTM!All four error paths in
GetDefaultIdentity()use appropriate sentinel errors with clear explanations and actionable hints. The multiple-defaults error (line 486) correctly includes the comma-separated list of defaults in context, matching the test expectations in manager_test.go.Also applies to: 464-469, 481-487, 497-502
1307-1313: LGTM!Identity lookup errors in both
GetEnvironmentVariables()andPrepareShellEnvironment()follow consistent patterns with appropriate sentinel errors and context.Also applies to: 1340-1346
1393-1400: LGTM!The
getProfiles()helper cleanly extracts profile context for error reporting. It properly handles nil stackInfo and returns nil, whichFormatProfile()correctly formats as "(not set)".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
errors/formatter.go (1)
418-453: Consistent with formatContextTable changes.The GetAllSafeDetails migration and nested iteration pattern match the changes in
formatContextTable, ensuring consistent context extraction across both rendering paths.Consider extracting the parsing logic (lines 427-437) into a shared helper function to reduce duplication with
formatContextTable(lines 71-81). Both functions parse the same "key=value" format identically.🔎 Optional refactor to reduce duplication
// parseContextKeyValues extracts key-value pairs from all safe details in the error chain. func parseContextKeyValues(err error) [][]string { allDetails := errors.GetAllSafeDetails(err) if len(allDetails) == 0 { return nil } var rows [][]string for _, layer := range allDetails { for _, detail := range layer.SafeDetails { str := fmt.Sprintf("%v", detail) pairs := strings.Split(str, " ") for _, pair := range pairs { if parts := strings.SplitN(pair, "=", 2); len(parts) == 2 { rows = append(rows, []string{parts[0], parts[1]}) } } } } return rows }Then both functions could call
parseContextKeyValues(err)instead of duplicating the parsing logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/auth_login.gocmd/terraform/utils.goerrors/formatter.gopkg/auth/hooks.go
🧰 Additional context used
📓 Path-based instructions (2)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Commands MUST use flags.NewStandardParser() for command-specific flags - NEVER call viper.BindEnv() or viper.BindPFlag() directly
Embed examples from cmd/markdown/*_usage.md using //go:embed and render with utils.PrintfMarkdown()
Files:
cmd/auth_login.gocmd/terraform/utils.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
cmd/auth_login.gopkg/auth/hooks.gocmd/terraform/utils.goerrors/formatter.go
🧠 Learnings (14)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/auth_login.gocmd/terraform/utils.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
cmd/auth_login.gopkg/auth/hooks.gocmd/terraform/utils.goerrors/formatter.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
pkg/auth/hooks.gocmd/terraform/utils.go
📚 Learning: 2024-11-24T19:13:10.287Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
cmd/terraform/utils.goerrors/formatter.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
cmd/terraform/utils.goerrors/formatter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
cmd/terraform/utils.goerrors/formatter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
cmd/terraform/utils.go
📚 Learning: 2025-12-13T03:21:27.620Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:27.620Z
Learning: In Atmos, when initializing CLI config via cfg.InitCliConfig, always first populate the ConfigAndStacksInfo struct with global flag values by calling flags.ParseGlobalFlags(cmd, v) before LoadConfig. LoadConfig (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) from the ConfigAndStacksInfo struct, not from Viper. Passing an empty struct causes the --base-path, --config, --config-path, and --profile flags to be ignored. Recommended pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
cmd/terraform/utils.go
🧬 Code graph analysis (2)
pkg/auth/hooks.go (2)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrNoDefaultIdentity(571-571)
cmd/terraform/utils.go (1)
errors/error_funcs.go (1)
CheckErrorPrintAndExit(318-360)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (5)
errors/formatter.go (1)
62-81: LGTM! Multi-layer context extraction working as intended.The switch to
GetAllSafeDetailscorrectly traverses the entire error chain, collecting context from all wrapped ErrorBuilder layers. The nested iteration properly handles the layered structure.Note: If the same key appears in multiple layers, it will be included multiple times in the table. This appears intentional for showing how context evolves through the error chain.
cmd/auth_login.go (1)
94-95: Good change - preserves ErrorBuilder context.Returning the error directly ensures hints and explanations from
GetDefaultIdentityreach the user intact. The previous wrapping would have stripped that rich context.cmd/terraform/utils.go (1)
336-337: Good change - preserves ErrorBuilder context for user-facing output.Passing the error directly to
CheckErrorPrintAndExitensures the explanations and hints are displayed. The previous wrapping would have buried the actionable guidance.pkg/auth/hooks.go (2)
95-96: Good change - preserves ErrorBuilder context.Returning the error directly ensures the rich context from
GetDefaultIdentityflows through to the caller.
99-103: Excellent use of the error builder pattern.The error construction is clean and actionable:
- Clear explanation of what went wrong
- Two specific hints guiding the user toward resolution
- Proper use of the sentinel error for
errors.Is()checksThis is exactly the kind of improvement the PR aims for.
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
github-actions
bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Update PRD to specify that full identity configuration in YAML format should only be displayed when --verbose flag is set. This keeps default error output concise while still providing detailed debugging info when needed. Changes: - R2 now specifies verbose-only display for identity config - Added behavior table showing default vs verbose mode - Updated examples to show both modes - Added helper function for verbose mode detection Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add appropriate language specifiers to code blocks for proper syntax highlighting: - Line 11: text for error example - Line 77: yaml for identity config - Lines 102, 108, 114: text for profile status examples Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove verbose mode and YAML identity config display - Use simple profile values (devops, not ATMOS_PROFILE=devops) - Standardize context keys: profile, provider, identity, expiration - Renumber requirements R1-R5 and examples 1-3 - Update helper function to FormatProfile with comma-separated output
… pattern - Add pkg/auth/errors.go with FormatProfile and FormatExpiration helpers - Migrate key auth errors in manager.go to use ErrorBuilder pattern - Add profile, provider, identity, and expiration context to auth errors - Add actionable hints for troubleshooting auth failures - Use sentinel errors (ErrAuthenticationFailed, ErrIdentityNotFound, etc.) - Update tests to account for new GetProviderName calls in error context Note: manager.go exceeds file-length-limit (915 lines vs 500 limit) but this is a pre-existing issue (837 lines on main). File refactoring is tracked separately. Implements: docs/prd/auth-error-messaging.md
The identity not found error now uses ErrorBuilder with structured context, providing a cleaner single-block error message with explanation and hints.
Update the remaining error paths in auth manager to use the ErrorBuilder pattern with context (profile, defaults) and actionable hints. Updated errors: - ErrIdentitySelectionRequiresTTY: Add TTY requirement explanation - ErrNoDefaultIdentity: Add hint for --identity flag and default config - ErrMultipleDefaultIdentities: Include list of conflicting defaults - ErrNoIdentitiesAvailable: Add hint for atmos.yaml configuration Updated test to use errors.Is() and GetAllSafeDetails() to verify error context instead of string matching.
The ErrDefaultIdentity wrapper in auth_login.go and terraform/utils.go was stripping the ErrorBuilder enrichments (hints, context, explanation) from GetDefaultIdentity errors. Changes: - cmd/auth_login.go: Return error directly instead of wrapping - cmd/terraform/utils.go: Pass error directly to CheckErrorPrintAndExit - pkg/auth/hooks.go: Return error directly, add ErrorBuilder for empty name
The formatter was using GetSafeDetails which only gets top-level safe details. ErrorBuilder wraps errors in multiple layers, so context was not being found. Changed to GetAllSafeDetails which traverses the entire error chain to find context at any level.
…parsing hooks.go: - Convert all user-facing errors to use ErrorBuilder pattern - Remove error wrapping that stripped ErrorBuilder context - Add profile context to all errors formatter.go: - Add parseContextPairs() to handle values with spaces (e.g., '(not set)') - The old parser split on spaces which broke values like 'profile=(not set)'
Convert all fmt.Errorf calls in auth manager files to use ErrorBuilder pattern with context keys (profile, provider, identity, expiration). Changes: - manager.go: Convert 20+ error sites to ErrorBuilder - manager_helpers.go: Convert auth not configured error - manager_logout.go: Convert identity/provider not found errors - manager_helpers_test.go: Update test to use errors.Is() Note: manager.go exceeds 500 line limit (pre-existing issue) Generated with Claude Code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update golden snapshot files to include the new ## Context section that ErrorBuilder errors now display. The ErrorBuilder changes to errors/formatter.go now use GetAllSafeDetails() to traverse the entire error chain, causing context information to be displayed for all errors that include it. Regenerated snapshots: - atmos_auth_whoami_--identity_nonexistent - terraform_plan_with_nested_component_path - terraform_plan_with_path_but_component_not_in_stack - describe_component_with_nested_component_path - describe_component_with_path_not_in_component_directory - atmos_terraform_plan_non-existent_in_non_workspace - atmos_workflow_no_steps - atmos_workflow_invalid_from_step Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/auth/hooks.go (1)
69-74: Duplicate error construction for auth manager creation failures.Both
TerraformPreHook(lines 69-74) andnewAuthManager(lines 196-201) construct nearly identical errors whenNewAuthManagerfails. SinceTerraformPreHookcallsnewAuthManager, the error from lines 196-201 will be wrapped again at lines 69-74, potentially duplicating context.Consider removing the wrapper at lines 69-74 and just returning the error directly:
🔎 Suggested simplification
authManager, err := newAuthManager(&authConfig, stackInfo) if err != nil { - return errUtils.Build(errUtils.ErrAuthManager). - WithCause(err). - WithExplanation("Failed to create auth manager"). - WithHint("Check your auth configuration in atmos.yaml"). - WithContext("profile", FormatProfile(stackInfo.ProfilesFromArg)). - Err() + return err }Also applies to: 196-201
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
errors/formatter.gopkg/auth/hooks.gopkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/manager_helpers_test.gopkg/auth/manager_logout.gotests/snapshots/TestCLICommands_atmos_auth_whoami_--identity_nonexistent.stderr.goldentests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_describe_component_with_nested_component_path.stderr.goldentests/snapshots/TestCLICommands_describe_component_with_path_not_in_component_directory.stderr.goldentests/snapshots/TestCLICommands_terraform_plan_with_nested_component_path.stderr.goldentests/snapshots/TestCLICommands_terraform_plan_with_path_but_component_not_in_stack.stderr.golden
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/TestCLICommands_terraform_plan_with_path_but_component_not_in_stack.stderr.golden
- tests/snapshots/TestCLICommands_describe_component_with_nested_component_path.stderr.golden
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/snapshots/TestCLICommands_atmos_auth_whoami_--identity_nonexistent.stderr.golden
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
pkg/auth/manager_logout.gopkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gopkg/auth/manager_helpers_test.goerrors/formatter.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Usenilif no atmosConfig param
Avoid adding new functions topkg/utils/; create purpose-built packages for new functionality (e.g.,pkg/store/,pkg/git/,pkg/pro/,pkg/filesystem/)
Files:
pkg/auth/manager_logout.gopkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gopkg/auth/manager_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
pkg/auth/manager_helpers_test.go
🧠 Learnings (42)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager_logout.gopkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gopkg/auth/manager_helpers_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/auth/manager_logout.gopkg/auth/manager.gopkg/auth/manager_helpers_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/manager_logout.gopkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gopkg/auth/manager_helpers_test.goerrors/formatter.go
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldentests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.goldentests/snapshots/TestCLICommands_terraform_plan_with_nested_component_path.stderr.golden
📚 Learning: 2025-02-14T23:12:38.030Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-02-14T23:12:38.030Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldentests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldenerrors/formatter.go
📚 Learning: 2025-10-11T19:12:38.832Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden:0-0
Timestamp: 2025-10-11T19:12:38.832Z
Learning: Usage Examples sections in error output are appropriate for command usage errors (incorrect syntax, missing arguments, invalid flags) but not for configuration validation errors (malformed workflow files, invalid settings in atmos.yaml). Configuration errors should focus on explaining what's wrong with the config, not command usage patterns.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldentests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldenpkg/auth/manager.gotests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldenpkg/auth/hooks.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.goldenerrors/formatter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gopkg/auth/manager_helpers_test.goerrors/formatter.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.gotests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/auth/manager.gopkg/auth/manager_helpers.gopkg/auth/hooks.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Configuration loading precedence: CLI flags → ENV vars → config files → defaults (use Viper). Environment variables require ATMOS_ prefix via `viper.BindEnv("ATMOS_VAR", ...)`
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Stack pipeline: Load atmos.yaml → process imports/inheritance → apply overrides → render templates → generate config. Templates use Go templates + Gomplate with `atmos.Component()`, `!terraform.state`, `!terraform.output`, store integration
Applied to files:
pkg/auth/manager_helpers.gopkg/auth/hooks.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/auth/manager_helpers.gotests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
pkg/auth/manager_helpers.goerrors/formatter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/auth/manager_helpers.gopkg/auth/hooks.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Applied to files:
tests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2025-12-24T04:29:23.938Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: pkg/ci/terraform/templates/apply.md:17-17
Timestamp: 2025-12-24T04:29:23.938Z
Learning: In the cloudposse/atmos repository, Terraform CI templates (pkg/ci/terraform/templates/*.md) are rendered using TerraformTemplateContext (defined in pkg/ci/terraform/context.go), not the base ci.TemplateContext. TerraformTemplateContext provides top-level fields: .Resources (ci.ResourceCounts), .HasChanges() method, and .HasDestroy field, which are correctly accessed directly in templates without a .Result prefix.
Applied to files:
tests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
tests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden
📚 Learning: 2024-12-03T03:52:02.524Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/terraform_utils.go:144-145
Timestamp: 2024-12-03T03:52:02.524Z
Learning: Avoid adding context timeouts to Terraform commands in `execTerraformOutput` because their execution time can vary from seconds to hours, and making it configurable would require redesigning the command interface.
Applied to files:
tests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.goldentests/snapshots/TestCLICommands_terraform_plan_with_nested_component_path.stderr.golden
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
pkg/auth/manager_helpers_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/auth/manager_helpers_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/auth/manager_helpers_test.go
🧬 Code graph analysis (3)
pkg/auth/manager_logout.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (2)
ErrIdentityNotInConfig(626-626)ErrProviderNotInConfig(627-627)pkg/auth/errors.go (1)
FormatProfile(10-15)
pkg/auth/manager_helpers.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrAuthNotConfigured(534-534)pkg/auth/errors.go (1)
FormatProfile(10-15)
pkg/auth/manager_helpers_test.go (3)
pkg/schema/schema_auth.go (1)
AuthConfig(4-12)pkg/auth/hooks.go (1)
Identity(22-22)errors/errors.go (1)
ErrAuthNotConfigured(534-534)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (23)
tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.golden (1)
12-17: Context section looks good.The addition of structured context (workflow name) is appropriate for this configuration validation error. It provides clear debugging information and aligns with the PR's ErrorBuilder pattern adoption across error types.
pkg/auth/manager_logout.go (2)
23-29: Good adoption of the ErrorBuilder pattern.The enriched error provides actionable hints and context. Using
FormatProfile(m.getProfiles())ensures consistent formatting across all auth errors.
120-126: Consistent ErrorBuilder usage for provider errors.Matches the pattern used for identity errors. The hints guide users to check available providers and review configuration.
pkg/auth/manager_helpers_test.go (2)
365-366: Correct migration to sentinel error assertions.Using
errors.Is(err, errUtils.ErrAuthNotConfigured)aligns with Go best practices and the coding guidelines. This approach is more robust than string matching.
339-356: Clean test case structure.The reformatted struct fields improve readability. Removing the
expectedErrorstring in favor of sentinel-based checks is the right approach.pkg/auth/manager_helpers.go (2)
253-259: ErrorBuilder provides clear guidance for missing auth config.The explanation and hints help users understand what's needed. Note that
FormatProfile(nil)correctly returns "(not set)" based on theFormatProfilehelper in errors.go.
276-298: Well-structured helper for stack auth loading.The function follows best practices:
- Has
perf.Trackfor observability- Handles errors gracefully without propagating
- Logs progress at appropriate levels
- Returns early on edge cases
The graceful error handling ensures stack loading issues don't block authentication.
pkg/auth/hooks.go (3)
33-42: Internal errors correctly flagged with issue reporting hint.For truly internal errors (nil stackInfo/atmosConfig), pointing users to the GitHub issues page is appropriate.
109-110: Preserving ErrorBuilder context by returning directly.Good call not to re-wrap. The comment clearly explains why the error is passed through unchanged.
113-117: Actionable hints for missing default identity.The two hints cover both immediate and configuration-based solutions.
pkg/auth/manager.go (8)
1556-1564: Clean helper for profile context extraction.The
getProfiles()method provides a nil-safe way to extract profiles from stackInfo. Used consistently across all error construction sites.
129-146: Consistent error enrichment for initialization failures.Provider and identity initialization errors now include profile context and actionable hints. The pattern is consistent across both initialization paths.
176-183: Authentication flow errors include full context.Each error in the authentication flow includes:
- Profile context
- Provider name (when available)
- Identity name
- Actionable troubleshooting hints
The provider name extraction logic at lines 191, 209-212 is sound.
Also applies to: 192-200, 208-220
344-392: GetCachedCredentials errors properly enriched.Both the "not found" and "expired" paths now include:
- Profile context
- Provider name (resolved from identity)
- Identity name
- Expiration time for expired credentials
Using
FormatExpiration(expTime)at line 391 ensures consistent expiration formatting.
454-501: GetDefaultIdentity errors cover all scenarios.The three scenarios (no TTY, no defaults, multiple defaults) each have appropriate error messages and hints. The "defaults" context at line 500 helps users identify which identities are conflicting.
1302-1329: Identity chain authentication errors include step context.The errors at lines 1302-1308 and 1316-1329 include the chain step information, which helps debugging complex identity chains.
1387-1407: Circular dependency and missing identity errors are clear.The circular dependency error (lines 1387-1393) and missing identity error (lines 1401-1407) both provide actionable guidance.
1417-1446: Via configuration validation errors are descriptive.The two error paths for missing/invalid via configuration (lines 1417-1423, 1440-1446) clearly explain what's wrong and how to fix it.
tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden (1)
17-23: LGTM! Context section enhances error diagnostics.The new Context table provides structured diagnostic data (from_step, workflow) that makes troubleshooting clearer. The formatting is consistent with the ErrorBuilder pattern introduced across the PR.
errors/formatter.go (1)
62-78: Strong implementation of multi-layer error context traversal.The switch from
GetSafeDetails()toGetAllSafeDetails()correctly captures context from the entire error chain. The nested iteration over layers and details ensures no context is lost when errors are wrapped at different levels. Clean separation of concerns between table formatting and markdown formatting.Also applies to: 414-449
tests/snapshots/TestCLICommands_describe_component_with_path_not_in_component_directory.stderr.golden (1)
18-26: Context section correctly displays multi-line values.The table formatting handles long paths by wrapping them across multiple lines while maintaining alignment. Good UX for deeply nested directory structures.
tests/snapshots/TestCLICommands_atmos_terraform_plan_non-existent_in_non_workspace.stderr.golden (1)
12-18: Handles empty context values gracefully.The Context table correctly displays
base_pathas empty rather than omitting it, which helps users distinguish between "not set" and "not applicable". Consistent with the other snapshot updates.tests/snapshots/TestCLICommands_terraform_plan_with_nested_component_path.stderr.golden (1)
16-24: Context data provides clear diagnostics for ambiguous paths.The structured context (match_count, matches, path_component, stack) gives users exactly what they need to understand why the path is ambiguous and how to fix it. Well-aligned with the hint suggesting to use the exact component name.
|
|
||
| // parseContextPairs parses a context string like "key1=value1 key2=value2" into key-value pairs. | ||
| // Values can contain spaces (e.g., "profile=(not set)"), so we parse by finding the next key= | ||
| // pattern rather than splitting on spaces. | ||
| func parseContextPairs(s string) [][]string { | ||
| var pairs [][]string | ||
|
|
||
| // Find all positions where a key starts (word followed by =). | ||
| // Pattern: space or start of string, followed by word characters, followed by =. | ||
| keyPattern := regexp.MustCompile(`(?:^|\s)([a-zA-Z_][a-zA-Z0-9_]*)=`) | ||
| matches := keyPattern.FindAllStringSubmatchIndex(s, -1) | ||
|
|
||
| for i, match := range matches { | ||
| // match[2] and match[3] are the start/end of the captured key group. | ||
| keyStart := match[2] | ||
| keyEnd := match[3] | ||
| key := s[keyStart:keyEnd] | ||
|
|
||
| // Value starts after the = sign. | ||
| valueStart := keyEnd + 1 // skip the = | ||
|
|
||
| // Value ends at the start of the next key or end of string. | ||
| var valueEnd int | ||
| if i+1 < len(matches) { | ||
| // Next match's full match starts at match[0], but we want the space before. | ||
| valueEnd = matches[i+1][0] | ||
| // Trim trailing space from value. | ||
| for valueEnd > valueStart && s[valueEnd-1] == ' ' { | ||
| valueEnd-- | ||
| } | ||
| } else { | ||
| valueEnd = len(s) | ||
| } | ||
|
|
||
| if valueStart <= valueEnd { | ||
| value := s[valueStart:valueEnd] | ||
| pairs = append(pairs, []string{key, value}) | ||
| } | ||
| } | ||
|
|
||
| return pairs | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the formatter.go file
find . -name "formatter.go" -type f | head -20Repository: cloudposse/atmos
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Look for test files related to errors package
fd -e test.go "test" errors/ 2>/dev/null || fd -e test.go . | grep -i error | head -10Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for parseContextPairs function and tests
rg -n "parseContextPairs" --type go -B 2 -A 15Repository: cloudposse/atmos
Length of output: 3064
🏁 Script executed:
#!/bin/bash
# Look for test files in errors directory
ls -la errors/ | grep -i testRepository: cloudposse/atmos
Length of output: 895
🏁 Script executed:
#!/bin/bash
# Search for parseContextPairs in test files
rg -n "parseContextPairs" --type goRepository: cloudposse/atmos
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Read the full parseContextPairs function and surrounding context
sed -n '559,605p' errors/formatter.goRepository: cloudposse/atmos
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Search for parseContextPairs tests in formatter_test.go
rg -n "parseContextPairs" errors/formatter_test.goRepository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for test functions that might test the context pairs functionality
rg -n "Test.*Context" errors/formatter_test.goRepository: cloudposse/atmos
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Get size of formatter_test.go and sample its content
wc -l errors/formatter_test.go
head -100 errors/formatter_test.goRepository: cloudposse/atmos
Length of output: 2577
🏁 Script executed:
#!/bin/bash
# Look at the context-related tests more closely
sed -n '210,270p' errors/formatter_test.goRepository: cloudposse/atmos
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Check if there are any inline tests or table-driven tests for parseContextPairs
rg -n "key.*value" errors/formatter_test.go -A 2 -B 2 | head -60Repository: cloudposse/atmos
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Check full TestFormatContextTable_WithContext test
sed -n '218,252p' errors/formatter_test.goRepository: cloudposse/atmos
Length of output: 1102
Implementation is correct, but add direct unit tests for edge cases.
The regex-based approach cleanly extracts key-value pairs by finding key boundaries rather than splitting on spaces, correctly handling values like profile=(not set). However, parseContextPairs has no direct unit tests. The existing context tests in formatter_test.go use the WithContext() API with separate arguments, not the string format this function parses. Add dedicated tests covering:
- Empty values:
key1= key2=value2 - Values containing equals signs:
key=value=something - Single pair:
key=value - Trailing/leading whitespace:
key=valueorkey1=value1 key2=value2
🤖 Prompt for AI Agents
In errors/formatter.go around lines 558 to 599, parseContextPairs is implemented
but lacks direct unit tests; add a new table-driven test in
errors/formatter_test.go that calls parseContextPairs directly and asserts
expected [][]string results for cases: empty value ("key1= key2=value2"), value
with equals ("key=value=something"), single pair ("key=value"), leading/trailing
whitespace (" key=value " and "key1=value1 key2=value2"); use the testing
package with clear subtests or table entries and compare lengths and exact
key/value strings, failing the test when any pair mismatches.
9325aa3 to
3580976
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
errors/formatter.go (1)
560-601: Add direct unit tests for parseContextPairs edge cases.As noted in previous review,
parseContextPairslacks direct unit tests. The existing context tests informatter_test.gouse theWithContext()API with separate arguments, not the string format this function parses. Add dedicated tests covering:
- Empty values:
key1= key2=value2- Values containing equals signs:
key=value=something- Single pair:
key=value- Trailing/leading whitespace:
key=valueorkey1=value1 key2=value2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
cmd/auth_login.gocmd/terraform/utils.godocs/prd/auth-error-messaging.mderrors/formatter.gopkg/auth/errors.gopkg/auth/errors_test.gopkg/auth/hooks.gopkg/auth/manager.gopkg/auth/manager_extended_test.gopkg/auth/manager_helpers.gopkg/auth/manager_helpers_test.gopkg/auth/manager_logout.gopkg/auth/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/auth/manager_logout.go
- pkg/auth/errors_test.go
- pkg/auth/manager_test.go
- cmd/terraform/utils.go
- pkg/auth/manager_helpers_test.go
- cmd/auth_login.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
pkg/auth/manager.gopkg/auth/errors.goerrors/formatter.gopkg/auth/manager_extended_test.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Usenilif no atmosConfig param
Avoid adding new functions topkg/utils/; create purpose-built packages for new functionality (e.g.,pkg/store/,pkg/git/,pkg/pro/,pkg/filesystem/)
Files:
pkg/auth/manager.gopkg/auth/errors.gopkg/auth/manager_extended_test.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
pkg/auth/manager_extended_test.go
🧠 Learnings (44)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform.go:37-46
Timestamp: 2025-01-18T15:15:41.645Z
Learning: In the atmos CLI, error handling is intentionally structured to use LogErrorAndExit for consistent error display, avoiding Cobra's default error handling to prevent duplicate error messages.
📚 Learning: 2025-12-13T06:07:34.794Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:34.794Z
Learning: For docs in the cloudposse/atmos repository under docs/prd/, markdownlint issues MD040, MD010, and MD034 should be deferred to a separate documentation cleanup commit and must not block the current PR. If needed, address these issues in a follow-up PR dedicated to documentation improvements.
Applied to files:
docs/prd/auth-error-messaging.md
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
docs/prd/auth-error-messaging.md
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
docs/prd/auth-error-messaging.mdpkg/auth/manager_helpers.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager.gopkg/auth/manager_extended_test.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/auth/manager.gopkg/auth/errors.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/auth/manager.gopkg/auth/errors.goerrors/formatter.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/auth/manager.gopkg/auth/errors.gopkg/auth/manager_helpers.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/auth/manager.goerrors/formatter.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Configuration loading precedence: CLI flags → ENV vars → config files → defaults (use Viper). Environment variables require ATMOS_ prefix via `viper.BindEnv("ATMOS_VAR", ...)`
Applied to files:
pkg/auth/manager.gopkg/auth/hooks.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/manager.gopkg/auth/errors.goerrors/formatter.gopkg/auth/manager_extended_test.gopkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
pkg/auth/errors.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to pkg/**/*.go : Avoid adding new functions to `pkg/utils/`; create purpose-built packages for new functionality (e.g., `pkg/store/`, `pkg/git/`, `pkg/pro/`, `pkg/filesystem/`)
Applied to files:
pkg/auth/errors.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go`; use `errors.Join` for combining multiple errors, `fmt.Errorf` with `%w` for adding string context, error builder for complex errors, and `errors.Is()` for error checking. Never use dynamic errors directly
Applied to files:
pkg/auth/errors.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/auth/errors.gopkg/auth/manager_helpers.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods following Go's documentation conventions
Applied to files:
pkg/auth/errors.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
errors/formatter.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
errors/formatter.gopkg/auth/manager_helpers.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
errors/formatter.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
errors/formatter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
errors/formatter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
errors/formatter.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
errors/formatter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
errors/formatter.go
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
errors/formatter.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Stack pipeline: Load atmos.yaml → process imports/inheritance → apply overrides → render templates → generate config. Templates use Go templates + Gomplate with `atmos.Component()`, `!terraform.state`, `!terraform.output`, store integration
Applied to files:
pkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/auth/hooks.gopkg/auth/manager_helpers.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
pkg/auth/hooks.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
pkg/auth/manager_helpers.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to pkg/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Use `nil` if no atmosConfig param
Applied to files:
pkg/auth/manager_helpers.go
🧬 Code graph analysis (2)
pkg/auth/hooks.go (4)
errors/builder.go (1)
Build(24-37)errors/errors.go (3)
ErrInvalidAuthConfig(565-565)ErrAuthManager(572-572)ErrNoDefaultIdentity(606-606)pkg/auth/errors.go (1)
FormatProfile(10-15)pkg/schema/schema_auth.go (1)
AuthConfig(4-12)
pkg/auth/manager_helpers.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrAuthNotConfigured(564-564)pkg/auth/errors.go (1)
FormatProfile(10-15)
🪛 LanguageTool
docs/prd/auth-error-messaging.md
[typographical] ~65-~65: Consider using a typographic opening quote here.
Context: ...---| | profile | Active profile(s) or "(not set)" | devops | | provider | P...
(EN_QUOTES)
[typographical] ~65-~65: Consider using a typographic close quote here.
Context: ...ofile| Active profile(s) or "(not set)" |devops| |provider` | Provider na...
(EN_QUOTES)
[typographical] ~90-~90: Consider using a typographic opening quote here.
Context: ...----|----------------| | Auth failure | "Run atmos auth --help for troubleshoot...
(EN_QUOTES)
[typographical] ~90-~90: Consider using a typographic close quote here.
Context: ... atmos auth --help for troubleshooting" | | Expired credentials | "Run `atmos a...
(EN_QUOTES)
[typographical] ~91-~91: Consider using a typographic opening quote here.
Context: ...ubleshooting" | | Expired credentials | "Run atmos auth login to refresh creden...
(EN_QUOTES)
[typographical] ~91-~91: Consider using a typographic close quote here.
Context: ...atmos auth loginto refresh credentials" | | Missing credentials | "Runatmos a...
(EN_QUOTES)
[typographical] ~92-~92: Consider using a typographic opening quote here.
Context: ... credentials" | | Missing credentials | "Run atmos auth login to authenticate" ...
(EN_QUOTES)
[typographical] ~92-~92: Consider using a typographic close quote here.
Context: ... "Run atmos auth login to authenticate" | | SSO session expired | "Run `atmos a...
(EN_QUOTES)
[typographical] ~93-~93: Consider using a typographic opening quote here.
Context: ...authenticate" | | SSO session expired | "Run atmos auth login --provider <name>...
(EN_QUOTES)
[typographical] ~93-~93: Consider using a typographic close quote here.
Context: ...in --provider to re-authenticate" | | Identity not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~94-~94: Consider using a typographic opening quote here.
Context: ...-authenticate" | | Identity not found | "Run atmos list identities to see avail...
(EN_QUOTES)
[typographical] ~94-~94: Consider using a typographic close quote here.
Context: ... identitiesto see available identities" | | Provider not found | "Runatmos li...
(EN_QUOTES)
[typographical] ~95-~95: Consider using a typographic opening quote here.
Context: ...le identities" | | Provider not found | "Run atmos list providers to see availa...
(EN_QUOTES)
[typographical] ~95-~95: Consider using a typographic close quote here.
Context: ...st providers` to see available providers" | | No profile set | "Set ATMOS_PROFILE...
(EN_QUOTES)
[typographical] ~96-~96: Consider using a typographic opening quote here.
Context: ...ailable providers" | | No profile set | "Set ATMOS_PROFILE or use --profile flag"...
(EN_QUOTES)
[typographical] ~96-~96: Consider using a typographic close quote here.
Context: ..."Set ATMOS_PROFILE or use --profile flag" | | Circular dependency | "Check identi...
(EN_QUOTES)
[typographical] ~97-~97: Consider using a typographic opening quote here.
Context: ...profile flag" | | Circular dependency | "Check identity chain configuration for c...
(EN_QUOTES)
[typographical] ~97-~97: Consider using a typographic close quote here.
Context: ... identity chain configuration for cycles" | ### R4: ErrorBuilder Migration All ...
(EN_QUOTES)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (13)
pkg/auth/manager_extended_test.go (1)
491-492: LGTM! Test expectation aligns with error context requirements.The mock expectation for
GetProviderName()correctly supports the enhanced error context introduced by the ErrorBuilder pattern. When expired credentials are encountered, the error now includes provider information for better diagnostics.pkg/auth/errors.go (1)
8-24: LGTM! Clean, focused error formatting helpers.Both
FormatProfileandFormatExpirationprovide clear formatting for error context display. The nil/empty handling is correct, and the RFC3339 format for timestamps is appropriate for structured error output.pkg/auth/manager_helpers.go (1)
276-299: LGTM! Well-structured helper for stack auth defaults.The
loadAndMergeStackAuthDefaultshelper is clean and focused. It correctly:
- Tracks performance with
perf.Track- Loads stack-level defaults
- Merges them with precedence over atmos.yaml
- Handles errors gracefully with debug logging
errors/formatter.go (2)
62-78: LGTM! Context traversal now covers full error chain.The update to use
GetAllSafeDetailscorrectly traverses the entire error chain, and the newparseContextPairsapproach handles values with spaces (like"profile=(not set)"). This enables the ErrorBuilder pattern to expose multi-layer context consistently.
417-435: LGTM! Markdown context formatting matches table approach.The markdown table generation correctly uses the same
GetAllSafeDetailstraversal andparseContextPairsparsing as the styled table formatter, ensuring consistent context display across output formats.pkg/auth/hooks.go (5)
33-42: LGTM! Consistent ErrorBuilder usage for validation errors.The nil checks for
stackInfoandatmosConfignow return structured errors with clear explanations and actionable hints to report internal errors. This improves debuggability over simple error wrapping.
69-74: LGTM! Auth manager creation errors now include rich context.Both auth manager initialization paths consistently use the ErrorBuilder pattern with:
- Cause preservation
- Clear explanations
- Actionable hints about configuration
- Profile context for troubleshooting
Also applies to: 196-201
92-97: LGTM! Decode errors now provide configuration guidance.The decode failure wraps the underlying error while adding explanation, hints, and profile context. This helps users troubleshoot auth configuration issues.
109-117: LGTM! Identity resolution errors are well-structured.Line 110 correctly returns the raw error (which already has ErrorBuilder context), while lines 113-117 construct a new structured error for the missing default identity case with clear hints about using
--identityflag or configuring defaults.
131-145: LGTM! Consistent error propagation in authentication flow.The raw error returns at lines 132 and 145 are correct - these underlying errors already contain ErrorBuilder context from
AuthenticateandPrepareShellEnvironment. Line 161-167 adds new builder context for the print failure case.pkg/auth/manager.go (2)
1607-1614: LGTM! Clean helper implementation.The
getProfiles()helper correctly encapsulates profile access for error context. Returns nil when stackInfo is unavailable, whichFormatProfile()handles gracefully by displaying "(not set)". This provides consistent profile context across all error paths.
129-135: Excellent ErrorBuilder pattern implementation.The comprehensive migration to ErrorBuilder provides rich, actionable error context throughout the auth manager. Key strengths:
- Consistent pattern: sentinel → cause → explanation → hints → context
- Profile context via
getProfiles()in every error path- Provider resolution with sensible fallbacks to "(not set)" when unavailable
- Actionable hints guide users toward resolution
- Context fields (profile, provider, identity, expiration) included appropriately
This significantly improves the developer experience when authentication fails, matching the PRD requirements exactly.
Also applies to: 165-170, 176-183, 248-255, 344-350, 365-372, 385-392, 600-607, 619-626, 1058-1065, 1174-1181, 1507-1513, 1567-1574
docs/prd/auth-error-messaging.md (1)
1-268: Well-structured PRD that guides implementation effectively.This PRD clearly defines requirements for improved auth error messaging with:
- Comprehensive context requirements (profile, provider, identity, expiration)
- Actionable hints table for each error type
- Clear examples showing desired user experience
- ErrorBuilder pattern migration guidelines
- Appropriate sentinel error categorization
The implementation in
manager.gofollows this guidance closely, providing consistent, helpful error messages throughout the auth subsystem.Based on learnings, markdownlint issues in PRD docs can be addressed in a separate documentation cleanup commit.
| return nil, errUtils.Build(errUtils.ErrAuthNotConfigured). | ||
| WithExplanation("Authentication requires at least one identity configured"). | ||
| WithHint("Add identity configuration to your atmos.yaml file"). | ||
| WithHint("Run `atmos auth --help` for configuration examples"). | ||
| WithContext("profile", FormatProfile(nil)). | ||
| WithContext("identity", resolvedIdentity). | ||
| Err() |
There was a problem hiding this comment.
Profile context is always "(not set)" - verify this is intentional.
At line 257, FormatProfile(nil) is called, which always returns "(not set)". Should this use FormatProfile(getProfiles()) or similar to show the actual profile context when available? The identity is resolved at this point, suggesting we have enough context to populate the profile field.
#!/bin/bash
# Verify if getProfiles() or similar method exists on manager type
rg -n "func.*getProfiles" pkg/auth/manager.go -A 5🤖 Prompt for AI Agents
In pkg/auth/manager_helpers.go around lines 253 to 259, the error return always
uses FormatProfile(nil) which yields "(not set)"; replace that nil with the
actual profiles from the manager so the logged profile context is meaningful —
call the manager's profile-fetching function (e.g.,
FormatProfile(m.getProfiles()) or FormatProfile(getProfiles()) depending on the
receiver/name in this file) so the error includes the real profile, not "(not
set)".
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
|
this is stale now, closing |
2 participants
Summary
Implements improved error messaging for
atmos authcommands to provide better developer experience when authentication fails.Changes:
pkg/auth/errors.gowithFormatProfile()andFormatExpiration()helperspkg/auth/manager.goto use ErrorBuilder patternProblem Statement
Previously, auth errors provided no visibility into why authentication failed:
Solution
Auth errors now include structured context and actionable hints:
Error Types Covered
Files Changed
pkg/auth/errors.gopkg/auth/errors_test.gopkg/auth/manager.gopkg/auth/manager_extended_test.gotests/snapshots/*.goldendocs/prd/auth-error-messaging.mdReferences
docs/prd/auth-error-messaging.md🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.